perf(Core): Reduce zero length array allocations#851
Open
Numpsy wants to merge 1 commit into
Open
Conversation
Numpsy
changed the title
Numpsy
force-pushed
the
reduce_empty_arrays
branch
from
749e33e to
093f1c2
Compare
knocte
force-pushed
the
reduce_empty_arrays
branch
from
093f1c2 to
0ca7b9e
Compare
Collaborator
|
@Numpsy LGTM, can you remove DRAFT status? Also I see a typo in commit msg "ruie" -> "rule" and now that you're going to fix it, please reduce the title (1st line) to max 50 chars, and make commit msg body same as PR description. Thanks |
Numpsy
force-pushed
the
reduce_empty_arrays
branch
from
0ca7b9e to
df5a7eb
Compare
Numpsy
changed the title
Contributor
Author
Do you want the benchmark numbers in the commit message, or just the description of the change? |
Numpsy
force-pushed
the
reduce_empty_arrays
branch
from
df5a7eb to
d93c0cc
Compare
Collaborator
Why leave any info outside the commit msg? If you do that, it will be lost when/if this repo moves to a different git hosting platform. When you add those, can you also:
With that, I'll merge. |
Numpsy
changed the title
Running the Visual Studio memory analyzer against the benchmarks project reveals that the current code is allocating a massive number of zero length arrays (over 3 million in that one benchmark). Many of these are from the Array functions in FSharp.Core always allocating a new empty array for an empty input. There are enough of these that avoiding some calls to Array.map with zero length inputs give a large reduction in memory allocations. Benchmarks with the current code: | Method | Mean | Error | StdDev | Gen0 | Gen1 | Gen2 | Allocated | |--------------- |---------:|--------:|--------:|-----------:|----------:|----------:|----------:| | LintParsedFile | 878.0 ms | 4.17 ms | 3.70 ms | 25000.0000 | 8000.0000 | 1000.0000 | 396.83 MB | Benchmarks with this change: | Method | Mean | Error | StdDev | Gen0 | Gen1 | Gen2 | Allocated | |--------------- |---------:|---------:|---------:|-----------:|----------:|----------:|----------:| | LintParsedFile | 865.0 ms | 12.17 ms | 10.79 ms | 20000.0000 | 7000.0000 | 1000.0000 | 321.69 MB |
Numpsy
force-pushed
the
reduce_empty_arrays
branch
from
d93c0cc to
24bd073
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…r to reduce the number of allocated zero length arrays.
As it stands, running the rules allocates a massive number of zero length arrays, which has quite a large effect on the amount of memory allocated. Many of these are from FSharp.Core functions which always create a new array, but which can be avoided with small local wrappers.
Just trying to get back to looking at some perf tests I started some time back, and found this one.
Running the Visual Studio memory profiler on the functions in the benchmarks project shows the allocations of massive numbers of zero length arrays (over 4 million in total). e.g.
If I run the benchmarks project with memory diagnosis enabled, I get this with the current code
Down to this with this change
If I enable all of the extra checks that are enabled in the 'run self checks' leg of the CI then the differences are even larger.
Before
After
These allocations are caused by FSHarp.Core liking to always create new arrays in various
Arrayfunctions, but to me it looks like it's a big enough gain to special case it locally?...Note: There are other places that could maybe be changed as well, and a similar situation with Array.collect, but I left it just doing one thing to get comments.